treewide: Add idma to ethernet #3

alex96295 · 2024-02-16T15:14:43Z

This PR adds idma to ethernet with an AXI stream interface

@chaoqun-liang This PR is a draft, and I will review it with @thommythomaso next week.

Note that there is a CI associated with it, that runs on this mirror (https://iis-git.ee.ethz.ch/github-mirror/pulp-ethernet). The CI runs your standalone test and elaboration in gf22 technology

First comments

The main issue to fix is the clock generation logic. Two main concerns on the current implementation:

The clock generation is handled internally. We prefer to expose the clock signals with different phases and handle clock generation logic in the system where ethernet is integrated~~
The way the eth clock is generated right now is behavioral and non-synthesizable (fll_dummy). Anyway, we are going to remove the clock generation logic and handle it outside of the IP, using clock dividers to get the clock frequency. Whether to use delay lines or clk_gen_hyper is up to discussion with the system's maintainers (for now: Al-Sqr and Cheshire).

There are (many) other smaller things that we will point out during the review

When this PR will be ready

We need to strengthen standalone verification (add random tests other then directed tests)
We need to have evidence of the IP working in-system via FPGA prototyping. This means:
Al-Saqr, on the FPGA where it is deployed
Cheshire, on the FPGA where it is deployed
We need to have a synthesizable design (run elab and compilation through the CI)

Since we already have directed verification for 1., we can focus on 2. for the moment. 3. is always important.

chaoqun-liang · 2024-02-20T10:18:18Z

internal clk gen removed : )

alex96295 · 2024-02-23T08:19:35Z

Bender.yml

+  common_verification : { git: "https://github.com/pulp-platform/common_verification.git", version: 0.2.3  }
+  register_interface : { git: "https://github.com/pulp-platform/register_interface.git",   version: 0.4.2  }
+  common_cells: { git: "https://github.com/pulp-platform/common_cells.git",                version: 1.32.0 }
+  idma: { git: "[email protected]:pulp-platform/iDMA.git",                                    rev: "a80fcac"  } # branch: cl/idma-eth


I guess we are pointing to main now, so update the comment

alex96295 · 2024-02-23T08:20:45Z

README.md

@@ -17,6 +17,12 @@ used.
 `pulp-ethernet` is intended for use with https://github.com/pulp-platform/ariane
 (a RISCV Linux-capable soft core).

+## Generate iDMA with AXIS support  (Terminal)


Change to

## Generate iDMA

since we generate it with all protocols

i also updated the make rules in the readme as i can see they are modified in the mk

alex96295 · 2024-02-23T08:27:57Z

gen/Makefile

@@ -18,18 +18,18 @@

 REGTOOL ?= regtool.py


Can we add also the commands to generate the register list as markdown file? This command should make it: https://github.com/pulp-platform/carfield/blob/main/carfield.mk#L236

thanks! i will include it in my next commit.

alex96295 · 2024-02-23T08:53:57Z

gen/eth_idma_reg.hjson

+              }
+          ]
+        },
+        { name: "src_addr",


I would prepend idma-specific registers with idma_

Also, design question to be discussed with @thommythomaso

AFAIU, this register file for ethernet-idma contains also the registers of the 'idma' part.

Since the idma already comes with its register file in different flavors, should we reuse that, and add the ethernet-specific registers as separate RF?

What do you think?

good point, i was suggested to have a unified reg file at the beggining

Replying to this before you change something: let's keep things like now (1 register file) :) in the future we can decouple and reuse the RF, now let's focus on functionality. I just wanted to point it out

sure. thanks!

alex96295 · 2024-02-23T08:55:15Z

rtl/eth_idma_pkg.sv

+// Solderpad Hardware License, Version 0.51, see LICENSE for details.
+// SPDX-License-Identifier: SHL-0.51
+
+


Missing authorship

// Authors: // blabla

i removed this eth_idma_pkg in another cleaner branch to leave the axi and register interface related typedef open to the users. [https://github.com/pulp-platform/pulp-ethernet/blob/cl/eth_idma/rtl/eth_idma_wrap.sv] check it out to see if it makes sense to you : )

As you prefer; for me, the less files the better, unless the information really needs to stay in a package for portability

understood. then i will remove it : )

alex96295 · 2024-02-23T08:57:52Z

rtl/eth_idma_pkg.sv

+package eth_idma_pkg;
+
+  /// Ethernet reg typedefs
+  parameter  int AW_REGBUS           = 32;


Naming conventions for tunable parameters according to the coding style-guide in System Verilog that we follow recommends using camel case: https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#summary-2

@chaoqun-liang can you please apply this to all parameters of this repo? I see that sometimes you do it, and sometimes you don't, needs to be consistent

alex96295 · 2024-02-23T09:03:10Z

rtl/eth_idma_pkg.sv

+  typedef struct packed {
+      axi_aw_chan_t aw_chan;
+      logic[max_width(axi_aw_chan_width, axis_t_chan_width)-axi_aw_chan_width:0] padding;
+  } axi_write_aw_chan_padded_t;


Can you explain why this padding is required? Is it to handle the case where axi stream width is larger than axi?

the padding type is required in the meta channel definition in idma backend.

alex96295 · 2024-02-23T09:05:16Z

rtl/eth_idma_pkg.sv

@@ -0,0 +1,102 @@
+// Copyright 2023 ETH Zurich and University of Bologna.


alex96295 · 2024-02-23T09:06:28Z

rtl/eth_idma_wrap.sv

+)(
+  input  logic                       clk_i,
+  input  logic                       rst_ni,
+  /// Etherent Internal clocks


Spelling typo, Ethernet

alex96295 · 2024-02-23T09:07:25Z

rtl/eth_idma_wrap.sv

+
+  logic  idma_req_valid, req_ready, idma_rsp_ready, rsp_valid;
+
+  localparam idma_pkg::error_cap_e ErrorCap = ErrorHandling ? ERROR_HANDLING : NO_ERROR_HANDLING;


What does Cap in ErrorCap mean?

according to idma_pkg, it stands for error handling capability

Can you add a comment please?

alex96295 · 2024-02-23T09:29:44Z

rtl/eth_top.sv

@@ -1,4 +1,4 @@
-// Copyright 2023 ETH Zurich and University of Bologna.
+/// Copyright 2023 ETH Zurich and University of Bologna.


Trailing /, remove

alex96295 · 2024-02-23T09:31:29Z

rtl/framing_top.sv

-// Authors:
-// - Jonathan Kimmitt <[email protected]>
-// - Thiemo Zaugg <[email protected]>
+// See LICENSE for license details.


Restore original license header and authors, append your name if you did modify the file

alex96295 · 2024-02-23T09:37:36Z

target/sim/src/eth_tb.sv

-  `AXI_STREAM_ASSIGN(rx_framing_slave_dv, rx_axis_rx_big);
+    // ------------------------ BEGINNING OF SIMULATION ------------------------
+
+   initial begin


You can use this module in common_verification to generate the clock: https://github.com/pulp-platform/common_verification/blob/master/src/clk_rst_gen.sv

thanks! i will use it to generate the in-phase 125Mhz. for the phase shifted one, i guess i need to stick with this.

alex96295 · 2024-02-23T09:38:04Z

target/sim/src/eth_tb.sv

+    // ------------------------ BEGINNING OF SIMULATION ------------------------
+
+   initial begin
+     while (!done) begin


why guarding this with while (!done)?

sorry that was from the original tb i started buidling on. forgot to remove it : (

alex96295 · 2024-02-23T09:42:20Z

@chaoqun-liang I left some initial (minor) comments

I'll give another round after we add the modifications to get it 'working' on FPGA

alex96295 · 2024-02-23T09:55:53Z

Bender.yml

-  common_cells: { git: "https://github.com/pulp-platform/common_cells.git", version: 1.32.0 }
-  axi_stream: { git: "[email protected]:pulp-platform/axi_stream.git", rev: "472751f550e3918215603e21734fe0ece3c66f79" }
+  axi : { git: "[email protected]:pulp-platform/axi.git",                                     version: 0.39.1 }
+  axi_mem_if : { git: [email protected]:pulp-platform/axi_mem_if.git,                         version: 0.2.1  }


If I am not mistaken, axi_mem_if is not required anymore; if so, please remove

If a "axi to mem" module is required, we have several in the axi repo (axi_to_mem_*), but iirc this was before adding the dma, when ethernet had buffers inside

alex96295 added 2 commits February 16, 2024 15:15

treewide: Add idma to ethernet

203445e

treewide: Mixed fixes

2af170c

alex96295 added the enhancement New feature or request label Feb 16, 2024

alex96295 assigned chaoqun-liang Feb 16, 2024

alex96295 requested a review from thommythomaso February 16, 2024 15:14

alex96295 and others added 8 commits February 16, 2024 16:17

eth.mk: Update nonfree pointer

92174e3

Update README.md

56737fb

eth.mk: Update nonfree pointer

a319bb6

Bender.yml: Use same IP commit lenght

f345e76

eth.mk: Update nonfree pointer

07bde79

target: Update keyword for succeeded test

0d15e42

restore external clk gen

e91ed1f

restore eth_top, tb update

94046fc

alex96295 commented Feb 23, 2024

View reviewed changes

chaoqun-liang added 2 commits February 26, 2024 17:47

multiple fixes

b7cef2f

clean up

7b6bf10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: Add idma to ethernet #3

treewide: Add idma to ethernet #3

alex96295 commented Feb 16, 2024 •

edited

Loading

chaoqun-liang commented Feb 20, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 26, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 26, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024 •

edited

Loading

alex96295 Feb 23, 2024 •

edited

Loading

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 26, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 Feb 23, 2024

chaoqun-liang Feb 26, 2024

alex96295 commented Feb 23, 2024

alex96295 Feb 23, 2024

alex96295 Feb 23, 2024

		// Solderpad Hardware License, Version 0.51, see LICENSE for details.
		// SPDX-License-Identifier: SHL-0.51

		@@ -0,0 +1,102 @@
		// Copyright 2023 ETH Zurich and University of Bologna.


		logic idma_req_valid, req_ready, idma_rsp_ready, rsp_valid;

		localparam idma_pkg::error_cap_e ErrorCap = ErrorHandling ? ERROR_HANDLING : NO_ERROR_HANDLING;

		@@ -1,4 +1,4 @@
		// Copyright 2023 ETH Zurich and University of Bologna.
		/// Copyright 2023 ETH Zurich and University of Bologna.

treewide: Add idma to ethernet #3

Are you sure you want to change the base?

treewide: Add idma to ethernet #3

Conversation

alex96295 commented Feb 16, 2024 • edited Loading

First comments

When this PR will be ready

chaoqun-liang commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex96295 Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

alex96295 Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex96295 commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex96295 commented Feb 16, 2024 •

edited

Loading

alex96295 Feb 23, 2024 •

edited

Loading

alex96295 Feb 23, 2024 •

edited

Loading